-
Notifications
You must be signed in to change notification settings - Fork 432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate to GitHub Actions #1073
Conversation
@newpavlov you may be able to help with this? |
Looks like we can use this for MUSL: https://github.com/NSCoder/rust-musl-action And for MIPS (or a BE target)... maybe use https://github.com/rust-embedded/cross |
I don't remember a good reason to test MUSL. Maybe to test portability of math results. MIPS is more important in that we need to test on a Big Endian target. So we don't necessarily need exactly the above targets. |
Looks good! The |
The MUSL target was introduced by @newpavlov for our 32-bit tests in 71d222c. I'm not sure why MUSL was chosen in particular. I agree that a Big Endian platform is important for testing. |
Looks good overall! I don't have experience of using VM (which IIUC will be needed for BE testing) with GitHub Actions, so I will not be able to help with that. Shouldn't we remove the Travis config completely or do you plan to keep it for now as a list of not ported tests? Also it may be useful to split tests of each crate into separate files. This way number of tests can be much smaller if only one crate gets affected. In RustCrypto we test each crate separately (see the
I don't remember exactly, but I probably had a problem with dynamically linked |
Yes.
I was assuming we'd be able to remove it completely in this PR, and probably There is a |
Any idea why only two variants of the test matrix get run? |
I call this a success for testing! Not entirely sure why it didn't fail before, but we didn't previously test |
Shouldn't the matrix look like this? matrix:
- os: [ubuntu-latest, windows-latest, macos-latest]
- toolchain: [stable, beta, nightly]
- include:
- os: windows-latest
toolchain: nightly
target: i686-unknown-linux-gnu
deps: sudo apt install gcc-multilib IIUC the |
Er... now the test matrix is empty?? |
I can't say at a glance why it works like this, but the simplified config in #1075 works as expected. Try to gradually extend it? |
According to the docs, the |
According to the Cargo manifest, this section is not used and the README should be used for build status.
Incomplete: Linux 32-bit, Big-Endian, Web
Hopefully the last changeset finally fixes the tests. @vks @newpavlov you might wish to review the switch to approximate testing in value stability. |
README.md
Outdated
@@ -104,8 +103,8 @@ greater, and 0.4 and 0.3 (since approx. June 2017) require Rustc version 1.15 or | |||
greater. Subsets of the Rand code may work with older Rust versions, but this is | |||
not supported. | |||
|
|||
Travis CI always has a build with a pinned version of Rustc matching the oldest | |||
supported Rust release. The current policy is that this can be updated in any | |||
Continuous Integration (CI) will always test the oldest supported Rustc version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe replace "oldest" with "minimum" for clarity?
I would prefer to define a macro for approximate testing: /// Assert that two numbers are almost equal to each other.
///
/// On panic, this macro will print the values of the expressions with their
/// debug representations.
#[macro_export]
macro_rules! assert_almost_eq {
($a:expr, $b:expr, $prec:expr) => (
let diff = ($a - $b).abs();
if diff > $prec {
panic!(format!(
"assertion failed: `abs(left - right) = {:.1e} < {:e}`, \
(left: `{}`, right: `{}`)",
diff, $prec, $a, $b));
}
);
} This gives a better error message than |
toolchain: nightly | ||
minimal: true | ||
variant: minimal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "minimal_versions" is clearer?
The |
rand_distr/src/normal.rs
Outdated
@@ -345,7 +345,8 @@ mod tests { | |||
assert_almost_eq!(lnorm.norm.std_dev, 1.0, 2e-16); | |||
|
|||
let lnorm = LogNormal::from_mean_cv(e.powf(1.5), (e - 1.0).sqrt()).unwrap(); | |||
assert_eq!((lnorm.norm.mean, lnorm.norm.std_dev), (1.0, 1.0)); | |||
assert!((lnorm.norm.mean - 1.0).abs() < 1e-15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one does not give a nice error message.
rand_distr/src/cauchy.rs
Outdated
@@ -160,7 +160,7 @@ mod test { | |||
let expected = [15.023088, -5.446413, 3.7092876, 3.112482]; | |||
for (a, b) in buf.iter().zip(expected.iter()) { | |||
let (a, b) = (*a, *b); | |||
assert!((a - b).abs() < 1e-6, "expected: {} = {}", a, b); | |||
assert!((a - b).abs() < 1e-5, "expected: {} = {}", a, b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also gives a worse error message: it does not tell the difference/error.
Your approach requires a lot of repetition and it is not very consistent. As far as I see it does not give the absolute error, which makes it harder to adjust the error threshold if a test is failing. |
Alright, I'll add your macro but keep the trait too. |
Turns out no new macro definitions are required... thus we obviously should have used this before! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good now!
Closes #1066
Incomplete: 32-bit Linux runner, Big-Endian runner, web (was already broken or removed but script left in utils/ci).
Fingers crossed most of this works...